Code Review

This pull request implements Slack Socket Mode support, enabling the adapter to handle events via a long-lived WebSocket. It includes a robust background reconnect loop with exponential backoff, logic for handling forwarded socket events in serverless setups, and updated configuration validation. Review feedback focuses on preventing a potential task leak during the connection phase and optimizing the backoff sleep mechanism by replacing the current polling approach with a more efficient asyncio.Event-based wait.

Code review — Slack Socket Mode port

Reviewed against upstream vercel/chat#162 (7e90d9c) on f55378a, applying docs/UPSTREAM_SYNC.md hazards #5/#6/#11/#12 and docs/SELF_REVIEW.md. Reviewed feat(slack): Socket Mode transport + the asyncio.Event follow-up. Net assessment: solid port with thoughtful Python-specific hardening. Two real issues worth tightening; the rest are nits.

🟡 Medium

• No connect timeout on SocketModeClient.connect() (hazard #11). start_socket_mode does await asyncio.wait({wait_task, self._socket_task}, return_when=FIRST_COMPLETED) with no timeout=. If the slack_sdk handshake hangs (DNS black-hole, TLS stall, mis-routed firewall), initialize() blocks forever and the caller has no observable signal. Upstream inherits the same problem from @slack/socket-mode, so this is parity, but docs/UPSTREAM_SYNC.md Hazard #11 explicitly flags "shared sessions plus explicit cleanup". Suggest wrapping the wait with asyncio.wait_for(..., timeout=connect_timeout_s) (configurable, default ~30s) and on timeout calling await self.stop_socket_mode() before raising. Add a regression test that injects a connect() that await asyncio.sleep(60)s.

• Forwarded events have no freshness check, even though the envelope ships a timestamp (hazard #12). _handle_forwarded_socket_event accepts any payload whose x-slack-socket-token matches. The forwarded JSON includes timestamp: Date.now() from the listener but neither side validates it. A leaked forwarding secret + replayed body would re-execute handlers indefinitely (and dedupe only catches identical message IDs, not re-injected modal submits). Upstream has the same gap; worth either (a) adding a 5-minute window check mirroring _verify_signature, or (b) documenting it in the non-parity table so future readers don't assume the timestamp is load-bearing. Same goes for the missing per-request nonce.

🔵 Nit

• stop_socket_mode suppresses Exception after await task (adapter.py:1413). with contextlib.suppress(asyncio.CancelledError, Exception): is broader than necessary — Exception covers everything that isn't a system-exiting BaseException, so a real bug surfaced post-cancellation is lost. The loop's except already logs; suggest narrowing to (asyncio.CancelledError,) and letting anything else log to stderr.

• asyncio.get_event_loop().create_task(...) in _route_socket_event.wrap_async (adapter.py:1568). Deprecated since 3.10 when no loop is set. Inside a running coroutine get_running_loop() is correct and matches start_socket_mode (which already uses it).

• _route_socket_event in interactive swallows dispatch errors then ack's empty (adapter.py:1672–1681). Upstream lets the error propagate to the slack_event listener; Python catches and ack's with no body. Net effect: a view_submission whose handler raises silently closes the modal instead of showing errors. Consider logging at error (already done) plus ack'ing with {"response_action": "errors", "errors": {"_": "internal error"}} so the user sees the modal didn't take.

• Cross-contamination test gap. TestSocketContextVar covers single-event isolation, but there's no test that fires two concurrent _route_socket_event(events_api) calls for different teams via asyncio.gather and asserts each handler sees the right token. Given the contextvars.copy_context() reliance (hazard #6) this is the exact regression class the doc warns about.

• Extra is_ext_shared_channel field added to the synthesized event_callback payload (adapter.py:1597) that upstream's routeSocketEvent doesn't include. Harmless additive, but if you want strict parity-on-the-wire, drop it (and the existing webhook path doesn't add it either, so this introduces a socket-vs-webhook asymmetry).

✅ Looks good

• app_token validated for xapp- prefix at construction; never logged. (Hazard #12)
• _verify_signature correctly refuses when _signing_secret is None so socket-mode adapters can't silently HMAC against "". (Hazard #12)
• hmac.compare_digest used for x-slack-socket-token. (Hazard #12)
• socket_forwarding_secret defaults to app_token matching upstream + lets ops override via env — the docstring even calls out the recommendation. (Hazard #12)
• _socket_task tracked, cancelled, awaited on shutdown. The wait_task orphan is now explicitly cancelled per the follow-up. (Hazard #5)
• _socket_shutdown_event (asyncio.Event) replaces the 250ms poll — _socket_sleep_with_backoff correctly uses wait_for; seconds=0 is well-defined (immediate TimeoutError) and unreachable here since _socket_initial_backoff_s = 1.0. (Hazard #5)
• Reconnect resets backoff = self._socket_initial_backoff_s after a successful connect; old SocketModeClient is disconnect()'d before the next iteration's SocketModeClient(...). No instance leak. (Hazard #11)
• Lazy import of slack_sdk.socket_mode.aiohttp inside start_socket_mode / _socket_mode_loop. (Hazard #10)
• mode="webhook" default preserves all existing webhook behavior; the dispatch path branches only on the x-slack-socket-token header and self._mode == "socket". Backwards-compatible.
• create_slack_adapter rejects mode="socket" + multi-workspace upfront — a real footgun that upstream silently allows.
• Outer reconnect layer + slack_sdk's auto_reconnect_enabled=True are documented divergence in the non-parity table; they don't fight because the outer loop only re-enters after the inner client is disconnect()'d and is_connected() returns False.

Posted by an automated reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

Generated by Claude Code

Re-review of e1c8047 ("address PR #86 review findings")

Verified fixes (7/7)

1. connect_timeout_s (default 30s) + wait_for + cleanup on timeout — Verified at src/chat_sdk/adapters/slack/adapter.py:256-258, 1399-1415. Regression test test_hung_connect_raises_timeout_and_cleans_up injects a 60s sleep and asserts _socket_task / _socket_client are nulled. test_connect_timeout_default_is_30s pins the default. PASS.
2. 5-min freshness on forwarded events + replay test + missing-timestamp test — Implemented at adapter.py:817-827; tests at tests/test_slack_socket_mode.py:782-820. PASS for the check shape, but see medium #M1 below — the units are wrong.
3. stop_socket_mode contextlib.suppress narrowed to (asyncio.CancelledError,) — adapter.py:1451-1452. PASS.
4. asyncio.get_running_loop() everywhere — grep confirms no get_event_loop() remains in the file (all four call sites use get_running_loop()). PASS.
5. Interactive dispatch error → response_action: errors ack — adapter.py:1721; regression test asserts body_arg.get("response_action") == "errors". PASS.
6. Concurrent cross-team asyncio.gather test — tests/test_slack_socket_mode.py:648-723; the slow first-team resolver (asyncio.sleep(0.02)) interleaves the second dispatch, so it would actually fail without copy_context() isolation. Real test, not phantom. PASS.
7. is_ext_shared_channel dropped from synthesized event_callback — adapter.py:1635-1641; test at :867-895. Matches upstream routeSocketEvent shape. PASS for parity, but see nit #N1.

uv run pytest tests/test_slack_socket_mode.py -q → 31 passed.

New findings

M1 — Forwarded-event timestamp wire format is incompatible with the upstream emitter (medium)

adapter.py:822 does abs(int(time.time()) - ts_int) > 300, treating ts_int as seconds. Upstream's forwardSocketEvent always serializes timestamp: Date.now() (packages/adapter-slack/src/index.ts:1837, 6188, 6217, 6244, 6285, 6314, 6353, 6393, 6537, 6584 — every test fixture confirms this) — i.e. milliseconds. A real JS-listener-emitted forwarded payload arrives with ts ≈ 1.78e12; the Python receiver computes abs(1.78e9 - 1.78e12) ≈ 1.78e12 > 300 and rejects with 401 every time, ~56,000 years of skew. The replay test only passes because it manually emits seconds (int(_time.time()) at tests/test_slack_socket_mode.py:296, 791). Since start_socket_mode_listener was intentionally not ported (docs/UPSTREAM_SYNC.md:498), the only expected producer is a JS process — which this rejects 100% of the time. Two viable fixes:

• Detect ms vs s by magnitude (if ts_int > 10**12: ts_int //= 1000) and update tests to also exercise the JS-shaped wire format, or
• Multiply the budget: > 300_000 and treat the field as ms.
  Either way, add a fidelity test that round-trips a literal Date.now()-shaped int (e.g. int(time.time() * 1000)).

M2 — Socket-mode loses external/shared-channel tracking (medium, latent upstream parity)

_process_event_payload at adapter.py:959 reads payload.get("is_ext_shared_channel") to populate _external_channels. The webhook path forwards the whole raw payload (so this works); the socket path now synthesizes a payload that omits the field (adapter.py:1635-1641). Downstream consumers of _external_channels (used for posting policies — _externalChannels at upstream 4359, 4449, 4787) silently never see socket-mode shared channels. Upstream has the same bug (its routeSocketEvent also drops it), so dropping the field matches upstream parity — but the previous reviewer's framing of this as a "fix" is misleading, and it is a real loss of functionality compared to the webhook path. Recommend either:

• Carrying is_ext_shared_channel through on the socket path as a Python-only divergence (tracked in docs/UPSTREAM_SYNC.md), or
• At minimum documenting the limitation in the socket-mode docs and the hazard table so it isn't quietly inherited at the next sync.

Nits

• N1 — The "fix" framing in commit e1c8047 for finding #7 reads as defense-in-depth, but the actual behavior change drops a field downstream code consumes. A one-line comment in _process_event_payload near the is_ext_shared_channel branch noting "socket-mode payloads never carry this; mirrors upstream routeSocketEvent" would prevent re-introducing it on a future port.
• N2 — _route_socket_event's 3 branches (events_api / slash / interactive) all add multi-workspace token resolution that upstream's routeSocketEvent doesn't have. This is reachable only by bypassing the factory (which rejects the combo at create_slack_adapter line 3729). Either remove the dead resolver paths in _route_socket_event (keep the factory guard as the single point of truth), or move the multi-workspace ValidationError into SlackAdapter.__init__ so the dead branches can be removed entirely.
• N3 — connect_timeout_s field default is 30.0 in SlackAdapterConfig (types.py:63), making the if config.connect_timeout_s is not None else 30.0 ternary at adapter.py:256-258 dead unless a caller explicitly passes None. Either drop the ternary or default the field to None.
• N4 — asyncio.wait_for wrapping a shielded asyncio.wait(..., FIRST_COMPLETED) (adapter.py:1399-1407) means the inner asyncio.wait keeps running after timeout. It currently completes naturally because stop_socket_mode() cancels _socket_task and the wait_task.cancel() resolves the wait_task — so it's safe, but a comment would help future readers (or replace the shield with explicit cancellation of _socket_task in the timeout branch).
• N5 — The slash_commands socket branch ignores the _handle_slash_command return value (matches upstream — slash responses go via response_url), but unlike upstream there's no wrapAsync(...).then(forwardToResponseUrl) wiring. This is consistent with how the webhook slash path also expects handlers to use response_url themselves, but worth a # slash responses are out-of-band via response_url, not the ack comment in _route_socket_event.

Upstream parity coverage hunted

• Lifecycle: disconnect() → stop_socket_mode() matches upstream disconnect(). PASS.
• Reconnect: Python adds an outer reconnect loop on top of slack_sdk's built-in auto_reconnect_enabled; upstream relies entirely on @slack/socket-mode's internal reconnect. Documented as belt-and-suspenders in UPSTREAM_SYNC.md:497. PASS.
• start_socket_mode_listener (transient serverless variant): intentionally not ported — UPSTREAM_SYNC.md:498. The forwarded-event receiver is ported, which is why M1 matters.
• Ack protocol: upstream-parity for events_api (empty), slash (empty), interactive (parsed JSON body or empty). PASS.
• Test fidelity vs upstream index.test.ts:5840-6600: all 5 factory-validation tests, all 4 socket-mode handleWebhook tests, all 6 routeSocketEvent dispatch tests, all 7 socket-mode-forwarding tests, the 3 startSocketModeListener tests (intentionally skipped per #498), and 3 routeSocketEvent-with-options tests are covered, plus 7 review-finding regressions. PASS.

Re-review verdict: FOLLOW-UP NEEDED — fixes 1, 3, 4, 5, 6, 7 are solid; fix 2 (forwarded-event freshness) is wired correctly but uses the wrong time unit and would reject every real upstream-emitted forwarded payload. M1 is a one-liner; M2 is a doc/decision item.

Posted by an automated re-reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

Generated by Claude Code